security: block plugin parser RCE and CLI argument injection#277
security: block plugin parser RCE and CLI argument injection#277advikdivekar wants to merge 7 commits into
Conversation
… filenames test(report): enhance filename assertions in SARIF report tests test(report): add tests for CSV, HTML, PDF, and SARIF report filename formats
…f_report The PDF route had generate_pdf_report called once outside the try block and again (wrapped in bytes()) inside it. The unguarded first call let any exception propagate past the _report_generation_error_response handler, causing CI to re-raise RuntimeError instead of returning a 500. Remove the duplicate call and the redundant bytes() wrapper; the single try-protected call is enough.
…ypass Three CRITICAL/HIGH vulnerabilities fixed (closes utksh1#265, utksh1#266, utksh1#267): 1. [CRITICAL] Vault: replace homemade XOR stream cipher (32-byte cycled keystream, trivially broken via crib-dragging) with AES-256-GCM using the cryptography package. Remove the hardcoded fallback key "secuscan-dev-key" — the app now raises at startup if SECUSCAN_VAULT_KEY is not set. Add cryptography>=42.0.0 to requirements.txt. 2. [HIGH] Auth: introduce startup-generated API key authentication (backend/secuscan/auth.py). On first run a 32-byte hex key is written to backend/data/.api_key and printed to the console. The key is required as "Authorization: Bearer <key>" or "X-Api-Key: <key>" on every route under /api/v1 via a router-level FastAPI dependency. The /api/v1/health and root endpoints remain unauthenticated for health probes. 3. [HIGH] Safe mode bypass: remove the overly broad "/" branch in is_filesystem_target() that classified CIDR notation (e.g. 8.8.8.8/32) and hostname+path strings (e.g. example.com/path) as local filesystem paths, causing validate_target() and all safe mode checks to be skipped. Only explicit filesystem roots (/, ./, ../, ~/, Windows C:\) now match.
Closes utksh1#202 — Plugin parser.py executed without integrity re-verification: - Add verify_parser_at_exec_time() to PluginManager that re-computes the combined metadata+parser digest immediately before exec_module() is called, closing the TOCTOU window between plugin load and parser execution. - When SECUSCAN_ENFORCE_PLUGIN_SIGNATURES=true, parsers with no checksum are refused outright; without enforcement a warning is logged. - executor.py calls this gate before the importlib exec path; on failure it falls through to built-in parsers rather than executing untrusted code. Closes utksh1#201 — Command argument injection via unsanitized scanner inputs: - Add _validate_inputs_against_schema() that enforces plugin field type constraints (select allowed-values, integer type, pattern regex) and calls _reject_injected_args() which blocks port specs containing flag prefixes (e.g. '--script=evil.nse') and rejects any other string field whose value begins with '-'. - Validation runs on raw inputs before _normalize_inputs() so select aliases like 'medium' are compared to declared options before path resolution. - Fix PortScanner.run() which passed '-sV' as scan_type (rejected by nmap plugin's select field) and '--top-ports 100' as ports (a flag, not a port range). Add _resolve_scan_type() and _resolve_ports() helpers that map convenience shorthands to schema-valid values. - Add 35 unit tests covering tampered-parser detection, enforcement mode, select/integer/pattern/injection validation, and PortScanner normalisation.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cd2e04f7c1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| from sse_starlette.sse import EventSourceResponse | ||
|
|
||
| router = APIRouter(prefix="/api/v1") | ||
| router = APIRouter(prefix="/api/v1", dependencies=[Depends(require_api_key)]) |
There was a problem hiding this comment.
Adding Depends(require_api_key) at router level makes every /api/v1/* endpoint return 401 unless callers send a key, but existing clients in this repo (for example frontend/src/api.ts and frontend/src/pages/Scans.tsx) do not attach Authorization or X-Api-Key headers at all. In practice this turns plugin listing, task start, status, reports, and SSE streaming into auth failures for the current UI and test callers, so the change is functionally breaking until a client-side key flow or route exemptions are added.
Useful? React with 👍 / 👎.
| Returns (scan_type_letter, service_detection_implied) where | ||
| service_detection_implied is True when the raw value requests version probing. | ||
| """ | ||
| raw = raw.strip().lstrip("-") |
There was a problem hiding this comment.
_resolve_scan_type and _resolve_ports call .strip() unconditionally, so requests that send {"scan_type": null} or {"ports": null} now raise AttributeError and fail the task before command construction. This is a regression from the previous behavior, where these values flowed through without crashing; the normalizer should coerce non-strings (or fall back to defaults) before stripping.
Useful? React with 👍 / 👎.
| if task_row["status"] not in ["completed", "failed"]: | ||
| raise HTTPException(status_code=400, detail="Task is not finished yet") | ||
|
|
||
| structured_data = json.loads(task_row["structured_json"]) if task_row["structured_json"] else {} |
There was a problem hiding this comment.
In the PDF download route, structured_json is parsed before the try block, so malformed historical rows now raise JSONDecodeError outside _report_generation_error_response and bypass the intended structured 500 response path used by the other report formats. Moving this parse back under the existing try keeps error handling consistent and prevents unexpected unhandled failures.
Useful? React with 👍 / 👎.
1 participant
What is the problem
Two critical vulnerabilities in the plugin execution pipeline:
#202 — Remote Code Execution via unverified parser.py execution
executor.py::_parse_results()dynamically loaded and executed each plugin'sparser.pyviaimportlib.exec_module()without re-verifying the file's integrity at execution time. The integrity check in_verify_plugin_integrity()runs only at plugin load time (startup or per-request in debug mode). Any modification toparser.pyafter that check — whether by a compromised process, a TOCTOU race, or a supply-chain edit — would be silently executed with full Python interpreter access. Additionally, withSECUSCAN_ENFORCE_PLUGIN_SIGNATURES=false(the default), plugins with no checksum at all were executed without any verification gate at execution time.#201 — Command argument injection via scan_type, ports, speed inputs
build_command()interpolated user-supplied string values directly into the CLI argument list with no type or content validation. Becauseasyncio.create_subprocess_execpasses each list element as a separateargventry, a value like--script=evil.nsesupplied for theportsfield would land in the subprocess argv as a standalone flag that tools like nmap interpret as--script. Additionally,PortScanner.run()was constructing nmap inputs with malformed values:scan_type="-sV"(rejected by the nmap plugin'sselectfield) andports="--top-ports 100"(a flag string, not a port range), both of which would themselves have triggered injection.What was changed
backend/secuscan/plugins.py_PORT_SPEC_PATTERNconstant:^[\d,\-]+$verify_parser_at_exec_time(plugin, plugin_dir): re-computes the combined metadata+parser digest withcompute_plugin_digest()immediately before execution and compares it to the stored checksum. Whenenforce_plugin_signatures=Trueand no checksum is present, execution is refused outright._validate_inputs_against_schema(plugin, inputs): enforces declared field types (select allowed-value set, integer parseability, regex patterns fromfield.validation.pattern) and calls_reject_injected_args()on allstring/textfields._reject_injected_args(field_id, value): forports/portfields, rejects any value not matching^[\d,\-]+$; for all other string fields, rejects values beginning with-.build_command()now calls_validate_inputs_against_schema()on raw inputs before_normalize_inputs()so that select-field alias checks see the original value, not the resolved path.backend/secuscan/executor.py_parse_results(), callsplugin_manager.verify_parser_at_exec_time()beforeexec_module(). On failure the custom parser is skipped and execution falls through to built-in parsers.backend/secuscan/scanners/port_scanner.py_resolve_scan_type(raw): maps nmap-style flags (-sV,-sS) and full strings to the single-letter codes (S,T,U) declared in the nmap plugin'sselectfield._resolve_ports(raw): maps convenience shorthands (top100,top1000,all) to numeric port ranges that pass the port-spec validator.speedfield; addedservice_detectionandos_detectionfields.testing/backend/unit/test_plugin_parser_security.py(new, 35 tests)Why this approach
The parser integrity check is placed at the point of execution rather than only at load time because load-time checks cannot prevent modification that occurs in the window between startup and when a task actually runs.
hmac.compare_digestprevents timing-side-channel leakage.Input validation fires before normalization in
build_command()so select-field values are compared against declared options before path resolution, and it covers every code path that builds a command.How to test
uvicorn backend.secuscan.main:app --reloadports="--script=malicious.nse"— expect HTTP 400 "Invalid port specification"scan_type="-sS --evil"— expect HTTP 400 "not in the allowed set"parser.pywhile the server runs, trigger a scan — observe "Parser integrity check FAILED" in logs and graceful fallback to built-in parsingSECUSCAN_ENFORCE_PLUGIN_SIGNATURES=true, removechecksumfrom a plugin's metadata — parser execution refused./testing/test_python.sh— all 35 new unit tests passEdge cases covered
--flag=valueor-flagprefix: rejected by port-spec pattern-: rejectedVerification
Labels:
type:security+level:critical+gssoc:approvedPlease assign this PR to me under GSSoC 2026.
Closes #201
Closes #202